Repository navigation
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
LeftHandCold
left a comment
There was a problem hiding this comment.
代码复核:建议通过,未发现需要 Request Changes 的确定性问题。审查 HEAD:267d535d1ff53d1d91cba6f6617b881ec17a1bd6。
设计判断:这是现有 legacy TEXT 恢复路径的局部解析性能修复,不是新增 schema 恢复能力或重构 catalog 所有权。缓存只记住确定性的语法证据,原解析器和恢复校验继续是唯一语义实现;无需另建通用缓存框架。对新增进程级状态单独审查了所有权、并发和边界。
逐项核对:
- exact SQL 为 key;解析仍枚举原有 SQL modes。仅成功解析入缓存,错误/歧义不缓存,超出限额则回到原解析路径。
- SQL/名称字符串和列切片由缓存复制持有,不保留 AST、TableDef、事务或解析器借用内存;现有消费者只读 evidence。
- 当前表身份、Seqnum、列兼容性、CREATE LIKE 的当前数据库/事务 resolver 每次重新检查。因此 DROP/ALTER/新快照不会直接复用旧 catalog 结论。
- 32 槽、SQL 16KiB、256 列、单条 64KiB 限制,保留 payload 至多 2MiB 加固定头部;读者持有的旧 entry 由普通 Go 引用生命周期释放。解析/分配在锁外,锁内仅有限数组查找/发布,没有后台 worker 或无界 miss registry。
- 测试包含 ownership、并发发布/淘汰、live LIKE resolver、identity/Seqnum、防歧义和超限回退。冷 miss 仍支付原 64-mode 解析成本,这是明确的取舍,不是所有 schema 场景都会加速。
验证边界:当前 HEAD 的 CI 40038653492 已成功,并核对其 UT 日志的 planner race 分片完成;diff --check 通过。本次未在本机重跑 planner/AgentLogs,PR 的真实数据收益是作者证据。
当前认证账号也是 PR 作者,故以 COMMENT 记录建议通过;这不是 GitHub 正式 Approval。
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: 267d535d1ff53d1d91cba6f6617b881ec17a1bd6
Assessment: approve
Reviewed all three changed files and frontend, internal SQL and dump/load consumers. The cache preserves the existing parser as the semantic owner and caches only detached syntax evidence; identity, Seqnum, compatibility and transaction resolution remain live. Bounded storage and short lock sections justify the added shared state. Correctness, ownership, concurrency and test-quality review found no actionable defects.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1517-7f90-b3f9-c5f9f9e7031b · slot 0 · generation 1
Validation:
-
Passed 16 focused normal-mode tests, 15 focused race tests and 100 repetitions of TestLegacyTextParseCacheConcurrentPublication. Receipts: 1792808002309272390-3.json, 1792807873389609685-3.json and 1792807909616244513-3.json.
-
Isolation preflight passed. Verified 192 native input files and accepted the recovered libraries through the current repository provenance checker.
-
A/B/B/A microbenchmarks used byte-exact merge-base recovery code and identical fixtures. Across six samples per variant, median warm recovery improved from 369.6 µs to 0.331 µs; warm LIKE improved from 482.8 µs to 7.45 µs. Cold misses added approximately 192 bytes and three allocations without observed timing slowdown. Plain-TEXT control timing varied by approximately 1%. Receipt: 1792808085688938917-3.json.
-
Reused supplied exact-head CI build, UT/coverage, static-analysis and BVT results for broader coverage.
-
Final audit confirmed unchanged source and reconciled commands, fingerprints, exits and retained logs. Receipt: 1792808180101459078-4.json. Evidence resides under /home/xupeng/.local/state/mo-bot/jobs/cc8335ca8c90f6ae76657781/attempt-1 in review-record.json and review-evidence-audit.json.
Limitations:
-
Author-reported AgentLogs measurements and the named delivery artifact were not independently reconciled. Local measurements establish recovery costs, not end-to-end SQL gains.
-
Full-package and SQL BVT checks were not repeated locally; broader coverage relies on the supplied exact-head CI snapshot. Per-case CI logs were unavailable.
-
The linked issue body was not supplied independently; its behavioral contract was assessed from the PR description.
|
Queued — the merge queue status continues in this comment ↓. |
Merge Queue Status
This pull request spent 14 minutes 25 seconds in the queue, with no time running CI. ReasonPull request #31008 has been dequeued Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Waiting for any of
All conditions
Tick the box to put this pull request back in the merge queue (same as
|
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #31007
What this PR does / why we need it:
Repeated plan validation on mixed TEXT/LONGTEXT tables reparses the same stored CREATE under 64 SQL modes. Reuse successful immutable syntax evidence in a bounded 32-slot cache. Keep current table identity, Seqnum, column compatibility and CREATE LIKE transaction/snapshot resolution live on every call. Errors and oversized inputs retain the original uncached path.
The cache owns detached SQL/name strings and column slices, at most 2 MiB of payload plus fixed headers. Parsing and allocation stay outside the lock; lookup/publication inspect at most 32 entries. No AST, table definition, transaction, goroutine or unbounded miss registry is retained.
On 998,799 complete AgentLogs records with the optional STORED layout, causal A/B/B/A (ten measured samples per version/query) changes Q03 from 8.206 to 0.810 ms, Q05 from 114.367 to 106.958 ms, and Q09 from 27.056 to 19.390 ms. Original-layout controls remain within 1.4% wall variation. All complete results/native metadata/physical identities pass; candidate full20 checks pass on both layouts. Four CPU cores, whole-engine/client/validator 12 GiB, no swap; measured peak 5.899 GiB and zero OOM/limit events. This isolates the cache on b787 plus the identical Group fix; it excludes later CONCAT and combined-patch gains.
Validation:
267d535d1fon main1651eec94875: full plan-package tests and four existing frontend table-dump recovery/schema/manifest consumers individually pass. Scoped vet/lint/gofmt/native provenance pass. Evidence:legacy-text-cache-delivery-main-closure-r1.json, SHAe924f8d906820e8f0f8c9ecd6565e7538765dcf7af15cffd0e7f4f79934db772.This draft changes three plan-package files only. It includes no benchmark layout, Group fix, schema format or storage migration. The separate Group prerequisite is needed by the benchmark fixture, not by this cache implementation.